Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(publisher-ers): Update ERS API usage for v2.x #3149

Merged

Conversation

ArekSredzki
Copy link
Contributor

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Recently, v2.0 of electron-release-server was released. This version updated many of the underlying packages and should be adopted by all users of the server. In the process, the API changed due to a change with SailsJS. This corrects the API usage within the ERS publisher plugin to allow users to continue using it.
Note that this issue was reported here: ArekSredzki/electron-release-server#314

@ArekSredzki ArekSredzki requested a review from a team as a code owner January 26, 2023 23:32
@ArekSredzki
Copy link
Contributor Author

@erickzhao Could you please review this change? :)

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! Does this change maintain backward compatibility with ERS 1.0, or will we need to add additional configuration to support both?

We also had an existing PR from a community member that attempted to fix this issue as well: #3124

The diff on that PR has a bunch of other changes. Can you confirm that those are not needed @ArekSredzki?

@ArekSredzki
Copy link
Contributor Author

Thanks @erickzhao, I wasn’t aware of that other PR, I must have missed the notification when I was mentioned. I’ll take a look through that PR now. Perhaps I’ve missed some necessary changes.

I’d like to drop support for v1.x if you’re okay with that since it’s very outdated. Updating the server is relatively easy and I’d encourage that all users do so.

@ArekSredzki ArekSredzki changed the title fix(publisher-ers): Update ERS API usage for v2.0 fix(publisher-ers): Update ERS API usage for v2.x Jan 27, 2023
@erickzhao
Copy link
Member

I guess I'm just hesitant to drop support right away in v6 of Forge and I feel like a breaking change should be reserved for v7.

Potentially keeping the old code path and deprecating it might be an alternative? Interested in what @electron/forgers think.

@ArekSredzki
Copy link
Contributor Author

I brought in most of the changes from the other PR since they were indeed desirable. However, I'd like to avoid adding additional logic to retain backward compatibility. If it's a must, would you be able to add that?

Recently, v2.0 of electron-release-server was released. This version updated many of the underlying packages and should be adopted by all users of the server. In the process, the API changed due to an change with SailsJS. This corrects the API usage within the ERS publisher plugin to allow users to continue using it.
Note that this issue was reported here: ArekSredzki/electron-release-server#314
@ArekSredzki ArekSredzki force-pushed the feature/fix-publish-ers-for-v2.0 branch from 2fd43d0 to 7fd414d Compare January 28, 2023 01:29
@ArekSredzki
Copy link
Contributor Author

ArekSredzki commented Jan 28, 2023

Update: I studied the V1.x API more carefully and have confirmed that these changes will work for v1 and v2 since v2 is more strict. As far as I can tell, there are actually some errors in the current mainline implementation since the v1.5.2 API always returns a version's flavor as an object and not a string (as v2 does).

@erickzhao
Copy link
Member

Alright, that's awesome then! Will take another look next week.

@ArekSredzki
Copy link
Contributor Author

@erickzhao Bump :)

@erickzhao erickzhao merged commit 3d5c87c into electron:main Feb 7, 2023
@ArekSredzki
Copy link
Contributor Author

Thanks!

@ArekSredzki ArekSredzki deleted the feature/fix-publish-ers-for-v2.0 branch February 8, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants